-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use genericclioptions package to handle kubeconfig #909
base: master
Are you sure you want to change the base?
Conversation
Use the same flagset that kubectl uses to select context and namespace, and generate the clientset from that. Fixes minio#884
I believe all codepaths to client init are adjusted now. |
Use the same flagset that kubectl uses to select context and namespace, and generate the clientset from that. Fixes minio#884
@@ -94,7 +95,9 @@ func init() { | |||
flag.Set("logtostderr", "true") | |||
flag.Set("alsologtostderr", "true") | |||
|
|||
mainCmd.PersistentFlags().StringVarP(&kubeconfig, "kubeconfig", "k", kubeconfig, "Path to the kubeconfig file to use for Kubernetes requests.") | |||
genericOptions = genericclioptions.NewConfigFlags(true) | |||
genericOptions.AddFlags(mainCmd.PersistentFlags()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work. We use flags which colliding with genericcliflags
. In fact we don't need support flags like kubectl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use flags which colliding with genericcliflags
Then use the flags from the flagset where they collide? I'm happy to work with you to identify where this is and see if it's trivial to access it via the flagset. I'm not sure if viper is able to access it as I haven't tested
In fact we don't need support flags like kubectl.
As referenced by #884, your tool does not work for those that are working with multiple kubeconfig files very easily. The flagset and its associated loaders provide a much more robust cluster and namespace selection, rather than having to juggle multiple kubeconfig directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use flags which colliding with genericcliflags
Apologies -- looking at the test output, it's not that you're using the same flags, there's reused shortnames. That is more of an issue.
It may be possible to rebuild just enough of the RESTClientGetter
interface to allow for cluster selection, without conflicting flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right way to fix the issue is to accept rest.Config
from the user. However this is not a priority now.
Use the same flagset that kubectl uses to select context and namespace, and generate the clientset from that.
Fixes #884